Skip to content

feat: Prepare Additional Conformity Test Scenarios (#3886) #3887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

mwb-al
Copy link
Contributor

@mwb-al mwb-al commented Jul 1, 2025

Description:

This PR refactors the conformity test suite (@conformity-batch-1) to improve code organization, readability, and maintainability. The main changes include:

What Was Removed

  • Redundant approaches for overriding Ethereum endpoints

What Was Added

  • New utils directory with modular utility functions:
    • utils.ts - Core utility functions
    • constants.ts - Test constants and configuration
    • interfaces.ts - Type definitions and interfaces
    • overwrites.ts - Override handling logic
    • processors.ts - Test data processing functions
    • validations.ts - Validation utilities
    • transactions.ts - Transaction-related utilities

Why These Changes Were Made

  1. Code Organization: Moved scattered test logic into organized, reusable modules
  2. Improved Readability: Separated concerns into logical modules
  3. Eliminated Redundancy: Unified the approach for handling Ethereum endpoint overrides (overwrites.ts)
  4. Better Maintainability: Modular structure makes it easier to maintain and extend tests

Validation Types in Override Files

The /overwrites folder contains various types of test validations:

eth_blockNumber

  • eth_blockNumber/simple-test.io - Simple block number test with wildcard result validation

eth_call

  • eth_call/call-callenv.io - Contract call with environment variables, wildcard result
  • eth_call/call-contract.io - Basic contract call test
  • eth_call/call-revert-abi-error.io - Contract call with ABI error revert
  • eth_call/call-revert-abi-panic.io - Contract call with ABI panic revert
  • eth_call/call-callenv-options-eip1559.io - Contract call with EIP-1559 options, wildcard result

eth_chainId

  • eth_chainId/get-chain-id.io - Chain ID retrieval test

eth_createAccessList

  • eth_createAccessList/create-al-contract.io - Create access list for contract interaction
  • eth_createAccessList/create-al-value-transfer.io - Create access list for value transfer
  • eth_createAccessList/create-al-contract-eip1559.io - Create access list for contract with EIP-1559

eth_estimateGas

  • eth_estimateGas/estimate-gas-contract.io - Gas estimation for contract calls
  • eth_estimateGas/estimate-gas-value-transfer.io - Gas estimation for value transfers
  • eth_estimateGas/estimate-gas-contract-eip1559.io - Gas estimation for EIP-1559 contracts

eth_feeHistory

  • eth_feeHistory/fee-history.io - Fee history with wildcards for gas ratios and base fees

eth_getBalance

  • eth_getBalance/get-balance-blockhash.io - Balance at specific block hash
  • eth_getBalance/get-balance.io - Standard balance retrieval
  • eth_getBalance/get-balance-unknown-account.io - Balance for non-existent account

eth_getBlockByHash

  • eth_getBlockByHash/get-block-by-hash.io - Block retrieval by hash

eth_getBlockByNumber

  • eth_getBlockByNumber/get-block-n.io - Block retrieval with multiple wildcard fields
  • eth_getBlockByNumber/get-finalized.io - Finalized block with hash and timestamp wildcards
  • eth_getBlockByNumber/get-latest.io - Latest block with dynamic fields
  • eth_getBlockByNumber/get-genesis.io - Genesis block with dynamic fields
  • eth_getBlockByNumber/get-safe.io - Safe block with wildcard result

eth_getBlockTransactionCountByNumber

  • eth_getBlockTransactionCountByNumber/get-genesis.io - Genesis block transaction count with wildcard

eth_getCode

  • eth_getCode/get-code-contract.io - Contract code retrieval
  • eth_getCode/get-code-empty.io - Empty contract code test

eth_getLogs

  • eth_getLogs/no-topics.io - Log query with invalid address parameter error
  • eth_getLogs/contract-addr.io - Log query with invalid topics parameter error

eth_getProof

  • eth_getProof/get-account-proof-blockhash.io - Account proof with unsupported method error

eth_getStorageAt

  • eth_getStorageAt/get-storage.io - Contract storage retrieval
  • eth_getStorageAt/get-storage-invalid-key.io - Invalid storage key error

eth_getTransactionByBlockHashAndIndex

  • eth_getTransactionByBlockHashAndIndex/get-transaction-by-block-hash-and-index.io - Transaction by block hash and index

eth_getTransactionByBlockNumberAndIndex

  • eth_getTransactionByBlockNumberAndIndex/get-transaction-by-block-number-and-index.io - Transaction by block number and index

eth_getTransactionByHash

  • eth_getTransactionByHash/get-transaction-by-hash.io - Transaction retrieval by hash

eth_getTransactionCount

  • eth_getTransactionCount/get-nonce.io - Account nonce retrieval

eth_getTransactionReceipt

  • eth_getTransactionReceipt/get-access-list.io - Transaction receipt for access list transactions
  • eth_getTransactionReceipt/get-blob-tx.io - Transaction receipt for blob transactions
  • eth_getTransactionReceipt/get-dynamic-fee.io - Transaction receipt for dynamic fee transactions
  • eth_getTransactionReceipt/get-legacy-contract.io - Transaction receipt for legacy contract transactions
  • eth_getTransactionReceipt/get-legacy-input.io - Transaction receipt for legacy input transactions
  • eth_getTransactionReceipt/get-legacy-receipt.io - Transaction receipt for legacy receipt transactions

eth_sendRawTransaction

  • eth_sendRawTransaction/send-access-list-transaction.io - Send access list transaction
  • eth_sendRawTransaction/send-blob-tx.io - Send blob transaction
  • eth_sendRawTransaction/send-dynamic-fee-access-list-transaction.io - Send dynamic fee access list transaction
  • eth_sendRawTransaction/send-dynamic-fee-transaction.io - Send dynamic fee transaction
  • eth_sendRawTransaction/send-legacy-transaction.io - Send legacy transaction

The list includes all endpoints that required modifications - even if only due to the fact that the transactions from chain.rlp are not replayed before the tests start - in order for them to pass successfully.

Related issue(s):
Fixes #3886

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

mwb-al added 19 commits July 1, 2025 14:35
…lob-related fields and improved wildcard handling (hiero-ledger#3886)

Signed-off-by: Michał Walczak <[email protected]>
…er handling and added overrides for enhanced modularity (hiero-ledger#3886)

Signed-off-by: Michał Walczak <[email protected]>
… expand transaction receipt test overrides (hiero-ledger#3886)

Signed-off-by: Michał Walczak <[email protected]>
@mwb-al mwb-al marked this pull request as ready for review July 7, 2025 14:28
@mwb-al mwb-al requested review from a team as code owners July 7, 2025 14:28
@mwb-al mwb-al requested a review from simzzz July 7, 2025 14:28
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff           @@
##             main    #3887   +/-   ##
=======================================
  Coverage        ?   48.12%           
=======================================
  Files           ?       83           
  Lines           ?     4819           
  Branches        ?      989           
=======================================
  Hits            ?     2319           
  Misses          ?     2122           
  Partials        ?      378           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

import { ErrorResponse, JsonRpcResponse, Method, Schema } from './interfaces';

// eslint-disable-next-line @typescript-eslint/no-var-requires
const execApisOpenRpcData = require('../../../../../../../openrpc_exec_apis.json');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this file doesn't exist yet when the CI is ran and it is causing it to fial, perhaps you can load it lazily here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: d1d61e1

Comment on lines 47 to 55
if (needError) {
return {
jsonrpc: '2.0',
id: request.id,
error: {
code: -32603,
message: error instanceof Error ? error.message : 'An unknown error occurred',
},
} as JsonRpcResponse;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return the actual error if such exists? e.g.

 if (error.response?.data) {
    return error.response.data;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: 3202fac

expect(valid).to.be.true;
if (response.result) {
console.log('Comparing response result with expected result.');
expect(response.result).to.be.equal(JSON.parse(content.response).result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: isn't this expect redundant because of the isResponseValid call from earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: b2d3aa7

// All other fields must remain unchanged to preserve the integrity of the original test case.

>> {"jsonrpc":"2.0","id":1,"method":"eth_getTransactionCount","params":["0xc37f417fA09933335240FCA72DD257BFBdE9C275","latest"]}
<< {"jsonrpc":"2.0","id":1,"result":"0x7"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on an exact nonce here could make the test fragile in the future, maybe we could use the wildcard here as well and only verify that the response is in a valid nonce format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: a31f265

@konstantinabl konstantinabl requested review from a team and konstantinabl July 10, 2025 08:15
@konstantinabl
Copy link
Contributor

konstantinabl commented Jul 10, 2025

Hi there, thanks for the great work!

This PR includes a large number of file changes and added lines, so it would be helpful to expand the description with more context.

Specifically:

Could you provide a summary of what was changed in the conformityTest.spec file (e.g., what was removed, what was added, and why) I see you created new files and exported functions, but you could just point this out in the description?

Also, please clarify why new tests were added—what functionality or edge cases are they covering?

It would be helpful to list the new tests briefly so reviewers can quickly understand their scope and purpose.

return checkObjectProperties(actual as Record<string, unknown>, expected as Record<string, unknown>, wildcards, path);
}

function checkValues(actual: unknown, expected: unknown, wildcards: string[], path = ''): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name for this is areValuesMatching or something like that, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: 2ef979a

return checkObjectProperties(actual as Record<string, unknown>, expected as Record<string, unknown>, wildcards, path);
}

function checkValues(actual: unknown, expected: unknown, wildcards: string[], path = ''): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add JSDoc for this method to better clarify what it does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: 2ef979a

import { legacyTransaction, transaction1559, transaction1559_2930, transaction2930 } from './transactions';
import { getTransactionCount } from './utils';

export async function updateRequestParams<T = unknown>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: db1d6f4

import { checkResponseFormat, findSchema, isResponseValid } from './validations';

export function splitReqAndRes(content: string) {
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the JSDoc inside the method, I know usually it is outside. May not be wrong, just wondering

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: 213fa2a

const ajv = new Ajv({ strict: false });
addFormats(ajv);

export function checkResponseFormat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used here https://github.com/hiero-ledger/hiero-json-rpc-relay/pull/3887/files#diff-91e05f05522a20e9c472bbe550e0d72948fe90f252ff853e9aed3ba95a90f833R63 where it is assigned to a variable 'hasMissingKeys' maybe its better to change the name to something like isFormatValid/hasResponseFormatIssues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: df81ef7


const ajv = new Ajv({ strict: false });
addFormats(ajv);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc for the checkResponseFormat method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: df81ef7

return false;
}

function checkComplexTypes(actual: object | null, expected: object, wildcards: string[], path: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again maybe the name is slightly misleading, since we return true for non-valid results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: a19ed69

@@ -23,5 +23,8 @@
// 6. tx.origin — 0x00 expected and returned; tx sent from 0x0, so origin is also zero.
// 7. msg.value — 0x00 expected and returned; no value was sent with the call, as expected.
//

## wildcard: result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwb-al So what exactly is the purpose of the wildcard, to not actually check the values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the description, most of these values (especially block.number, block.coinbase, difficulty/prevrandao) vary with each run and chain state. Hence, the wildcard is required to ensure the test remains valid.

mwb-al added 9 commits July 11, 2025 10:34
…th detailed parameter and operation descriptions (hiero-ledger#3886)

Signed-off-by: Michał Walczak <[email protected]>
…update references with improved JSDoc documentation (hiero-ledger#3886)

Signed-off-by: Michał Walczak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare Additional Conformity Test Scenarios for Execution API
3 participants